fix: Enable union type support in @opentelemetry/instrumentation-graphql (#1506) #2923
Conversation
nitrofski
changed the title
nitrofski
changed the title
|
Hi @nitrofski and thanks for your contribution. I order to proceed we need a couple of things:
Cheers |
|
@obecny can you please take a look at this? |
|
I merged with main. There are issues with CLA approval at my company at the moment, which we are working on getting resolved. I will sign as soon as that's done. Sorry for the delay. |
|
CLA issues have been resolved. |
|
@obecny can you please take a look at this? |
|
Good day, This hasn't seen feedback in over a month. Is there anything I can do to help get attention on this PR? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2923 +/- ##
=======================================
Coverage 91.44% 91.45%
=======================================
Files 146 146
Lines 8194 8202 +8
Branches 1846 1848 +2
=======================================
+ Hits 7493 7501 +8
Misses 701 701
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| @@ -305,11 +305,7 @@ export function wrapFields( | |||
| tracer: api.Tracer, | |||
| getConfig: () => GraphQLInstrumentationParsedConfig | |||
| ): void { | |||
| if ( | |||
| !type || | |||
| typeof type.getFields !== 'function' || | |||
There was a problem hiding this comment.
I'm not familiar enough with this library. Are we certain that all possible values of type have a function named getFields?
There was a problem hiding this comment.
It is part of graphqlTypes.GraphQLObjectType. As long as we maintain type safety and that the type definitions are accurate (to my testing, they were), we should be safe.
The previous implementation used an any type before recursing, so they lost type safety and ended up passing non-compliant object. It had to re-assess the object here.
|
|
||
| // unwrap union types | ||
| if (isGraphQLUnionType(type)) { | ||
| return type.getTypes(); |
There was a problem hiding this comment.
Here I have a similar question. Are we certain that getTypes() returns an array of GraphQLObjectType. Could be possible to have nested unions?
There was a problem hiding this comment.
Fair concern. Doing a quick search through the graphql type definitions, GraphQLUnionType is the only type with a getTypes() function, and it always returns an array of GraphQLObjectType. That all being said, is it enough for us or do we want to be extra safe and also validate what the function returns at runtime?
There was a problem hiding this comment.
Could be possible to have nested unions?
Doing a bit of research...
From the Apollo Graphql Union and Interfaces schema help page
All of a union's included types must be object types (not scalars, input types, etc.). Included types do not need to share any fields.
It sounds like unions strictly require their components to be objects and that nested union are not allowed. Though, to dispel any doubts, I tried launching my graphql server with a dummy union type that uses other union types:
Union type MyTestUnion can only include Object types, it cannot include <other union type>.
There was a problem hiding this comment.
thanks for your thorough explanation @nitrofski :)
|
@nitrofski thank for resolving my doubts. You have my approval but I'll wait a bit in case someone else wants to do a review. @obecny maybe? |
Which problem is this PR solving?
Fixes #1506.
Short description of the changes
Enable support for installing field instrumentation on union types.
The "type unwrap" logic was generalized to support unwrapping to a list of types. It is now a recursive function rather than a loop. The new unwrapping has been implemented in a way that is more agreeable to TypeScript.